Use session.detach for SDK session cleanup - #2307

Pull request overview

Updates SDK session cleanup to use ownership-aware session.detach, preserving shared and resumable sessions.

Changes:

• Replaces session destruction with detach across six SDKs.
• Validates detach responses and updates cleanup routing.
• Adds lifecycle and multi-client regression coverage.

Review details

• Files reviewed: 27/27 changed files
• Comments generated: 3
• Review effort level: Balanced

Generated by SDK Consistency Review Agent for #2307 · sonnet46 123.4 AIC · ⌖ 6.01 AIC · ⊞ 6.6K

Comments that could not be inline-anchored

private ensureConnected(): void {
    if (this.disconnected) {
        throw new Error(`Session ${this.sessionId} has been disconnected`);
    }
}

Generated by SDK Consistency Review Agent for #2307 · sonnet46 48.5 AIC · ⌖ 5.54 AIC · ⊞ 6.6K

I reproduced the real persistence bug: after actual conversation state is written, main's session.destroy path prevents a fresh client from resuming by explicit session ID, while this branch's session.detach path preserves and resumes it. I also added real-runtime cold-resume E2E coverage across all six SDKs using the shared replay snapshot, and validated the Node and .NET targeted E2Es locally. Rust test compilation passes with cargo check --tests; the full Rust E2E run was killed by the sandbox during compilation, not by a test failure. Python/Go/Java targeted runs were not available in this environment due missing toolchains/runners.

I reproduced the persistence bug and completed the PR on current main. The branch now uses session.detach for SDK cleanup across Node, Python, Go, .NET, Java, and Rust, and adds a real-runtime cold-resume E2E test in all six SDKs with a shared deterministic replay snapshot.

Re-approving after the current-main rewrite fix. The branch remains scoped to replacing cleanup-time session.destroy with session.detach across SDKs and adding real-runtime cold-resume E2E coverage.

Re-approving after fixing the Node in-process fake CLI detach response. The branch is mergeable and the change remains the same scoped detach cleanup plus real-runtime cold-resume coverage.

Re-approving after fixing Java and Node fake-runtime detach responses and Java formatting. The implementation still preserves the original scope: cleanup-time detach across all SDKs plus cold-resume E2E coverage.

Re-approving after updating the language fake runtimes to return the new session.detach success payload. The real runtime behavior and scope remain unchanged.

Re-approving after the final test-fake/expectation updates for detach. Rust test compilation passes locally; hosted CI should now validate the full matrix.

Fixed a real Go regression found in the port: Disconnect() was returning early on session.detach RPC failure before running local cleanup (event processing stop, GitHub token provider release, handler map clearing), unlike the original session.destroy path which always cleaned up locally regardless of RPC outcome. Restored unconditional local cleanup; only the returned error depends on RPC outcome. Verified locally: go build ./..., go vet ./..., gofmt -l . clean, and 148/148 relevant Go unit tests pass including the previously-failing TestGitHubTokenProviderCleanupOnDisconnectError.

Fixed the remaining Java InProcess/JDK test failure: the TimeoutEdgeCaseTest fake RPC stream's response-matching buffer wasn't reset after non-matching (hanging) requests, so a later session.detach request's response could be sent with a stale, already-consumed request's id, causing a spurious 5s timeout in close(). Fixed by always resetting the buffer after each flush (each sendMessage call flushes exactly once). Verified locally end-to-end with a real JDK 17 + JDK 25 Maven toolchain (downloaded fresh in this sandbox): mvn spotless:check and the full mvn test suite (all modules, including the new cold-resume E2E test) now pass with zero failures.